ENH: Add "Trailing Return Types" section by N-Dekker · Pull Request #162 · InsightSoftwareConsortium/ITKSoftwareGuide

N-Dekker · 2021-10-06T21:15:51Z

Adds a guideline in accordance with commit InsightSoftwareConsortium/ITK@c54ad67
"COMP: Use trailing return type instead of typename + dependent type"

Discussed at pull request InsightSoftwareConsortium/ITK#2780
"Workaround VS2017 compile errors using-declarations"

Adds a guideline in accordance with commit InsightSoftwareConsortium/ITK@c54ad67 "COMP: Use trailing return type instead of typename + dependent type" Discussed at pull request InsightSoftwareConsortium/ITK#2780 "Workaround VS2017 compile errors using-declarations"

Modernized template member function definitions in "*.hxx" files by using trailing return types, specifically for return types that are themselves specified in terms of a member type, dependent on the specific class template. By Notepad++ v8.1.4, Find in Files: Find what: ^typename (\w+<.+>::)(.+)\r\n\1(.+)\r\n{\r\n Replace with: auto\r\n$1$3 -> $2\r\n{\r\n Filter: itk*.hxx [v] Match case (*) Regular expression Following ITK commit InsightSoftwareConsortium/ITK@c54ad67 Anticipating ITK SoftwareGuide pull request InsightSoftwareConsortium/ITKSoftwareGuide#162 "ENH: Add "Trailing Return Types" section" Manually fixed `elx::TransformBase::GenerateDeformationFieldImage(void)` by adding a `typename` keyword.

Modernized template member function definitions by using trailing return types, specifically for return types that are themselves specified in terms of a member type, dependent on the specific class template. By Notepad++ v8.1.4, Find in Files: Find what: ^typename (\w+<.+>::)(.+)\r\n\1(.+)\r\n{\r\n Replace with: auto\r\n$1$3 -> $2\r\n{\r\n Filter: itk*.hxx [v] Match case (*) Regular expression Following ITK commit InsightSoftwareConsortium/ITK@c54ad67 Anticipating ITK SoftwareGuide pull request InsightSoftwareConsortium/ITKSoftwareGuide#162 "ENH: Add "Trailing Return Types" section" Manually fixed `elx::TransformBase::GenerateDeformationFieldImage(void)` by adding a `typename` keyword.

thewtex

👍 💇

jhlegarreta

Thanks for doing this.

Can the clang style tool enforce these rules?

jhlegarreta · 2021-10-20T20:19:51Z

SoftwareGuide/Latex/Appendices/CodingStyleGuide.tex

+
+Whenever choosing between a trailing return type (as introduced with C++11) and
+the old form of having the return type before the function name (as was already
+supported by C++98), prefer the form that is the shortest, and the simplest.


I would prefer avoiding the "simplest" and "shortest" terms/would prefer a clear/objective rule: e.g. when using language built-in types, use the return type before the function name.

A few reasons:

I do not think contributors will be computing the length of the added characters/saying language built-in types is more clear IMO

If the ITK PR was done automatically using a regex, the rules of that regex should apply here (i.e. there was no explicit modelling of "shortest"/"simplest").

N-Dekker · 2021-10-21T08:25:02Z

Thanks for your comments so far @thewtex and @jhlegarreta

For the record: an example of the VS2017 compile error that occurs when using the old (C++98) form for return types is here: InsightSoftwareConsortium/ITK#2759 (comment) Specifically:

template <typename T>
typename Derived<T>::NestedType
Derived<T>::MemberFunction()
{  // <==  error C2244: unable to match function definition to an existing declaration

Such an error can be avoided by using trailing return types.

Clang-tidy has two very different options:

modernize-use-trailing-return-type: "Rewrites function signatures to use a trailing return type"
fuchsia-trailing-return: "Functions that have trailing returns are disallowed, except for those using decltype specifiers and lambda with otherwise unutterable return types"

Interestingly fuchsia-trailing-return cannot be used, as long as we need to support VS2017.

I wouldn't mind some delay for this pull request. As long as we carefully avoid those specific VS2017 errors, we may also just continue doing what we always did, without a strict trailing-return-type rule.

jhlegarreta · 2021-10-21T13:22:19Z

Interestingly fuchsia-trailing-return cannot be used, as long as we need to support VS2017.

Not sure I see why.

As long as we carefully avoid those specific VS2017 errors,

I guess there is no guarantee for that and we have to rely on the CDash builds to spot those.

without a strict trailing-return-type rule.

IMO not having a tool to automatically do this will make the code inconsistent. e.g. think already to all the remotes, even more so when some of their parts are integrated into ITK. Although we as reviewers may spot the appearances, we are very, very likely to miss them, even more so not being used to using them.

If fuchsia-trailing-return cannot be used, then a pre-commit hook with the used regex may be enough for now.

N-Dekker · 2021-10-21T14:06:27Z

Interestingly fuchsia-trailing-return cannot be used, as long as we need to support VS2017.

Not sure I see why.

The fixes we committed to ITK for those VS2107 compile errors would be rejected by fuchsia-trailing-return. For example, looking at InsightSoftwareConsortium/ITK#2759 (comment) the following trailing-return-type is disallowed by fuchsia-trailing-return:

template <typename T>
auto
Derived<T>::MemberFunction() -> NestedType

But we do use trailing-return-types in such cases, to avoid those VS2017 errors! Catch-22!

So maybe we could postpone a decision on a trailing-return-type guideline for ITK until we have dropped VS2017, eventually 😺

Or otherwise we could do Clang-Tidy modernize-use-trailing-return-type! Some C++ users very much like to have trailing-return-types everywhere, as it is the most consistent approach.

jhlegarreta · 2021-10-21T14:30:45Z

But we do use trailing-return-types in such cases, to avoid those VS2017 errors! Catch-22!

OK.

So maybe we could postpone a decision on a trailing-return-type guideline for ITK until we have dropped VS2017, eventually smiley_cat

~~If the ITK PR gets merged, this one will need to be merged. Guidelines need to go hand in hand with the changes.~~

Using the Clang-Tidy trailing return type modification should be discussed in the ITK PR. If people are not for it, a pre-commit hook is the only solution for now as I see it.

Edit: I now realize that the ITK PR got merged a few days ago, sorry. I think the pre-commit hook would allow us to keep consistency.

dzenanz · 2021-12-07T21:51:57Z

Is this ready for "official review" and merging?

N-Dekker · 2021-12-08T11:10:58Z

Is this ready for "official review" and merging?

@dzenanz It is open for discussion, but Jon (@jhlegarreta) and I could not really get to an agreement. And honestly there appears no consensus amongst experienced C++ programmers in general, on whether to use trailing return types "whenever possible", or only "whenever necessary".

My proposal is still:

Whenever choosing between a trailing return type (as introduced with C++11) and the old form of having the return type before the function name (as was already supported by C++98), prefer the form that is the shortest, and the simplest.

Unfortunately, I'm not aware of any tool that automatically checks the code against my proposed rule.

Currently, ITK follows a similar (!) but slightly different rule: Use a trailing return type when it is necessary to avoid those VS2017 compilation errors. This rule is checked automatically at the dashboard, from time to time 😸

If we can't get to an agreement, it's OK to me to close the PR without merging.

dzenanz · 2021-12-08T14:49:06Z

Whenever choosing between a trailing return type (as introduced with C++11) and the old form of having the return type before the function name (as was already supported by C++98), prefer the form that is the shortest, and the simplest.

I agree with this. At least for the new code. If changing the old code is a manual work, we should not do it.

The trailing return types have access to the class' scope, and all its ~~typedefs~~ aliases, which frequently results in shorter type specification, not to mention avoidance of redundancy.

dzenanz · 2021-12-08T14:50:13Z

If both trailing and leading return type specifications have about the same length, we should prefer the classic (leading) one.

jhlegarreta · 2021-12-08T14:50:25Z

While the proposal and the merged style/compilation changes in InsightSoftwareConsortium/ITK@c54ad67 look valuable to me, as mentioned in this comment #162 (comment) adopting one style or the other is not enforced and, more importantly, relies on some requirement that is hard to fulfill, that is, computing the number of characters, or a subjective perception of what is "shortest/"simplest", or is ultimately upon the contributors' (or reviewers') commitment.

As said, if the InsightSoftwareConsortium/ITK@c54ad67 change could be formalized into a script (which may require some work, but I'd dare to say looks definitely doable), and that into a pre-commit hook, that would make things more consistent, and I'd definitely be for the change.

Also, note that ITK has 51 remote modules. It is not an easy task to keep the ITK changes in sync with them. Having automated ways to do so eases the task, scales easily (e.g. to future remote modules), and reduces the workload when the modules make their way into proper ITK.

Hope this makes sense.

dzenanz · 2021-12-08T14:54:01Z

We could formalize it like this, then:

Return type specification must be used if the return type references any of the type aliases of the class, and the classic (leading) type specification would require redundant repeat of their definition in the specification itself. Otherwise, the classic type specification must be used.

N-Dekker · 2021-12-08T17:50:44Z

@dzenanz Thanks for presenting a formalization, but personally I think the PR is clear enough. It also includes two examples, to clarify the rule:

In the following example, the old form is preferred over a trailing return type:

// Preferred:
template <typename T1, typename T2>
bool
AlmostEquals(T1 x1, T2 x2)

// Rather than:
template <typename T1, typename T2>
auto
AlmostEquals(T1 x1, T2 x2) -> bool

In the following example, a trailing return type is preferred over the old form:

// Preferred:
template <typename TValue, unsigned int VLength>
auto
FixedArray<TValue, VLength>::Size() const -> SizeType

// Rather than:
template <typename TValue, unsigned int VLength>
typename FixedArray<TValue, VLength>::SizeType
FixedArray<TValue, VLength>::Size() const

That's OK, right?

I think the main question is: do we want such a rule, or not? It won't be the end of the world, either way.

dzenanz

This section is short and clear enough. I vote in favor of this, for new code. If anyone wants to update the old code to the new style, that is welcome.

hjmjohnson

I think this is a sufficient step towards describing the intent of these proposed code changes.

The details and contention seem to be trying to find a more perfect solution for potential future problems. I am approving with the confidence that if some of those potential future problems materialize that this community will address those in an appropriate way.

Did Notepad++ v8.4.8, Find in Files (Regular expression enabled): Find what: ^typename (\w+<.+>::)(.+)\r\n\1(.+)\r\n{\r\n Replace with: auto\r\n$1$3 -> $2\r\n{\r\n Find what: ^inline typename (\w+<.+>::)(.+)\r\n\1(.+)\r\n{\r\n Replace with: inline auto\r\n$1$3 -> $2\r\n{\r\n Specifically for "itk*.h" files In accordance with ITKSoftwareGuide pull request InsightSoftwareConsortium/ITKSoftwareGuide#162 commit InsightSoftwareConsortium/ITKSoftwareGuide@c64b23b "ENH: Add "Trailing Return Types" section" Follow-up to pull request InsightSoftwareConsortium#4026 commit f048a5b "STYLE: Use trailing return type for declarations containing "inline""

Replaced `typename` with trailing return types in multi-line template member function declarations. Did so by Notepad++ v8.4.8, Find in Files: Find what: ^typename (\w+<.+>::)(.+)\r\n\1((.+\r\n)+?){\r\n Replace with: auto\r\n$1$3 -> $2\r\n{\r\n Filters: itk*.hxx;itk*.h;itk*.cxx [v] Match case (*) Regular expression Manually fixed two `CopyDisplacementField` member functions, which still needed a `typename` keyword anyway. In accordance with ITKSoftwareGuide pull request InsightSoftwareConsortium/ITKSoftwareGuide#162 commit InsightSoftwareConsortium/ITKSoftwareGuide@c64b23b "ENH: Add "Trailing Return Types" section" Follow-up to pull request InsightSoftwareConsortium#4026 commit f048a5b "STYLE: Use trailing return type for declarations containing "inline""

Did Notepad++ v8.4.8, Find in Files (Regular expression enabled): Find what: ^typename (\w+<.+>::)(.+)\r\n\1(.+)\r\n{\r\n Replace with: auto\r\n$1$3 -> $2\r\n{\r\n Find what: ^inline typename (\w+<.+>::)(.+)\r\n\1(.+)\r\n{\r\n Replace with: inline auto\r\n$1$3 -> $2\r\n{\r\n Specifically for "itk*.h" files In accordance with ITKSoftwareGuide pull request InsightSoftwareConsortium/ITKSoftwareGuide#162 commit InsightSoftwareConsortium/ITKSoftwareGuide@c64b23b "ENH: Add "Trailing Return Types" section" Follow-up to pull request #4026 commit f048a5b "STYLE: Use trailing return type for declarations containing "inline""

Replaced `typename` with trailing return types in multi-line template member function declarations. Did so by Notepad++ v8.4.8, Find in Files: Find what: ^typename (\w+<.+>::)(.+)\r\n\1((.+\r\n)+?){\r\n Replace with: auto\r\n$1$3 -> $2\r\n{\r\n Filters: itk*.hxx;itk*.h;itk*.cxx [v] Match case (*) Regular expression Manually fixed two `CopyDisplacementField` member functions, which still needed a `typename` keyword anyway. In accordance with ITKSoftwareGuide pull request InsightSoftwareConsortium/ITKSoftwareGuide#162 commit InsightSoftwareConsortium/ITKSoftwareGuide@c64b23b "ENH: Add "Trailing Return Types" section" Follow-up to pull request #4026 commit f048a5b "STYLE: Use trailing return type for declarations containing "inline""

N-Dekker marked this pull request as ready for review October 7, 2021 12:00

N-Dekker mentioned this pull request Oct 9, 2021

COMP: Use trailing return type instead of typename + dependent type SuperElastix/elastix#532

Merged

thewtex approved these changes Oct 20, 2021

View reviewed changes

thewtex requested review from hjmjohnson and jhlegarreta October 20, 2021 20:02

jhlegarreta reviewed Oct 20, 2021

View reviewed changes

N-Dekker marked this pull request as draft October 27, 2021 17:55

dzenanz requested a review from blowekamp December 8, 2021 14:50

N-Dekker marked this pull request as ready for review December 8, 2021 18:06

dzenanz approved these changes Dec 8, 2021

View reviewed changes

hjmjohnson approved these changes Dec 8, 2021

View reviewed changes

dzenanz approved these changes Jan 18, 2022

View reviewed changes

dzenanz merged commit c64b23b into InsightSoftwareConsortium:master Jan 18, 2022

N-Dekker mentioned this pull request Apr 21, 2023

STYLE: Change some declarations to use trailing return type InsightSoftwareConsortium/ITK#4020

Merged

2 tasks

N-Dekker mentioned this pull request Apr 28, 2023

Use trailing return type instead of typename in "*.h" files and multi-line member function declarations InsightSoftwareConsortium/ITK#4032

Merged

Conversation

N-Dekker commented Oct 6, 2021

Uh oh!

thewtex left a comment

Choose a reason for hiding this comment

Uh oh!

jhlegarreta left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhlegarreta Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

N-Dekker commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhlegarreta commented Oct 21, 2021

Uh oh!

N-Dekker commented Oct 21, 2021

Uh oh!

jhlegarreta commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dzenanz commented Dec 7, 2021

Uh oh!

N-Dekker commented Dec 8, 2021

Uh oh!

dzenanz commented Dec 8, 2021

Uh oh!

dzenanz commented Dec 8, 2021

Uh oh!

jhlegarreta commented Dec 8, 2021

Uh oh!

dzenanz commented Dec 8, 2021

Uh oh!

N-Dekker commented Dec 8, 2021

Uh oh!

dzenanz left a comment

Choose a reason for hiding this comment

Uh oh!

hjmjohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jhlegarreta left a comment •

edited

Loading

jhlegarreta Oct 20, 2021 •

edited

Loading

N-Dekker commented Oct 21, 2021 •

edited

Loading

jhlegarreta commented Oct 21, 2021 •

edited

Loading